Skip to content

Expand peer registry usage and improve reputation system for catchup #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 88 commits into from
Nov 11, 2025
Merged

Expand peer registry usage and improve reputation system for catchup #20

merged 88 commits into from
Nov 11, 2025

Conversation

@galt-tr
Copy link
Contributor

@galt-tr galt-tr commented Oct 17, 2025 •
edited
Loading

This pull request introduces a new catchup status reporting feature to the BlockValidation service and its HTTP API, along with internal refactoring to support peer tracking and P2P client integration. Additionally, it updates several generated protobuf files due to a protoc version change.

Catchup Status Reporting and API Integration:

  • Added a new HTTP endpoint (/catchup/status) in the asset service that queries the BlockValidation service for real-time catchup status, including peer, progress, and error details. (services/asset/httpimpl/GetCatchupStatus.go [1] services/asset/httpimpl/http.go [2]
  • Implemented GetCatchupStatus gRPC method in the BlockValidation server to provide detailed catchup progress and previous attempt information for monitoring and dashboards. (services/blockvalidation/Server.go services/blockvalidation/Server.goR442-R496)

BlockValidation Service Refactoring and P2P Client Integration:

  • Refactored the BlockValidation server to use a p2pClient for peer registry and reporting, replacing the previous peer metrics structure, and updated its constructor and daemon startup logic to inject this dependency. (daemon/daemon_services.go [1] [2]; services/blockvalidation/Server.go [3] [4] [5] [6] [7]
  • Updated peer tracking comments and logic to clarify the use of the P2P service for peer identification. (services/blockvalidation/Server.go [1] [2]

Testing and Mocking Enhancements:

Protobuf and Dependency Updates:

  • Regenerated multiple protobuf files to reflect a protoc version downgrade from 6.33.0 to 6.32.1, ensuring compatibility and consistency across services. (errors/error.pb.go [1] model/model.pb.go [2] services/alert/alert_api/alert_api.pb.go [3] services/alert/alert_api/alert_api_grpc.pb.go [4] services/asset/asset_api/asset_api.pb.go [5] services/blockassembly/blockassembly_api/blockassembly_api.pb.go [6] services/blockassembly/blockassembly_api/blockassembly_api_grpc.pb.go [7] services/blockchain/blockchain_api/blockchain_api.pb.go [8] services/blockchain/blockchain_api/blockchain_api_grpc.pb.go [9]

Other Improvements:

These changes collectively improve the observability, extensibility, and reliability of the catchup and block validation processes, while also modernizing peer tracking and supporting infrastructure.

References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22] [23] [24]

@galt-tr galt-tr force-pushed the peerRegistryViewer branch 3 times, most recently from 70a538b to c409711 Compare October 27, 2025 13:00
@galt-tr galt-tr force-pushed the peerRegistryViewer branch 2 times, most recently from 5c9f984 to 07fe262 Compare October 29, 2025 18:42
@galt-tr galt-tr marked this pull request as ready for review October 29, 2025 18:54
@galt-tr galt-tr force-pushed the peerRegistryViewer branch from 07fe262 to 78c3f3f Compare October 29, 2025 19:08
@galt-tr galt-tr closed this Oct 29, 2025
@galt-tr galt-tr reopened this Oct 29, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2025 •
edited
Loading

🤖 Claude Code Review

Status: Complete

This PR adds catchup status reporting and peer registry integration to the BlockValidation service. The implementation is functional but several architectural concerns raised by reviewers need addressing:

Key Issues from Review:

  1. Architecture - P2P Client in Asset HTTP Handlers (icellan comments #2486330509, #2486334523)

    • services/asset/httpimpl/get_peers.go and get_catchup_status.go access clients via repository
    • Per project patterns, clients should be passed from daemon during initialization
    • Repository should focus on data access, not service client management

  2. Interface Location (icellan comment #2486382996)

    • services/blockvalidation/p2p_client_interface.go should be in the p2p package
    • While it helps avoid circular deps, consider if blockvalidation needs its own subset interface

  3. Secret Mining Detection Logic (icellan comments #2486363812, #2486374354)

    • Lines 1032-1037 in catchup.go: Using utxoStore.GetBlockHeight() for main chain
    • Should check the fork chain height being caught up to, not main chain
    • Current logic may incorrectly flag legitimate forks as secret mining

  4. Common Ancestor Height Check (icellan comment #2486359158)

    • Lines 363-380 in catchup.go: Complex UTXO height validation during ancestor finding
    • Purpose and edge cases should be clarified - why filter by UTXO height vs blockchain store?

  5. Test Change Clarification (icellan comment #2486343910)

    • Line 414 in BlockValidation_error_test.go: Direct field assignment to invalidBlockKafkaProducer
    • Bypasses normal initialization - needs explanation of why this test pattern changed

  6. Fork Manager Usage (icellan comment #2486379812)

    • Is fork manager still actively used? If deprecated, should be noted

No Critical Bugs Found:

The P2P methods (IsPeerMalicious, IsPeerUnhealthy) mentioned as "missing" are actually implemented in services/p2p/handle_catchup_metrics.go:226-338. The P2P client is properly cached in daemon/daemon_stores.go:110.

Recommendation:

Focus on addressing the architectural feedback about client initialization patterns and the secret mining detection logic. The functionality appears sound but the structure should align better with project conventions.

github-actions[bot]
github-actions bot reviewed Oct 30, 2025
View reviewed changes
github-actions[bot]
github-actions bot reviewed Oct 30, 2025
View reviewed changes
@galt-tr

This comment was marked as outdated.

Sign in to view
@galt-tr galt-tr changed the title Enable support for viewing peer registry details Expand peer registry usage and improve reputation system for catchup Oct 30, 2025
@bsv-blockchain bsv-blockchain deleted a comment from galt-tr Oct 30, 2025
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
galt-tr
galt-tr commented Oct 30, 2025
View reviewed changes
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 5, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
46.1% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

icellan
icellan requested changes Nov 6, 2025
View reviewed changes
services/blockvalidation/catchup.go

if commonAncestorMeta.Invalid {
return errors.NewProcessingError("[catchup][%s] common ancestor %s at height %d is marked invalid, not catching up", catchupCtx.blockUpTo.Hash().String(), commonAncestorHash.String(), commonAncestorMeta.Height)
return errors.NewBlockInvalidError("[catchup][%s] common ancestor %s at height %d is marked invalid, not catching up", catchupCtx.blockUpTo.Hash().String(), commonAncestorHash.String(), commonAncestorMeta.Height)
Copy link
Contributor

@icellan icellan Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this trigger a reputation hit? I don't think it should, since the block is valid, but the previous block is invalid. That's why it previously was NewProcessingError

oskarszoon and others added 9 commits November 6, 2025 13:16
@oskarszoon oskarszoon merged commit dbe6202 into bsv-blockchain:main Nov 11, 2025
10 checks passed
@oskarszoon oskarszoon mentioned this pull request Nov 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@icellan icellan icellan approved these changes

@ordishs ordishs Awaiting requested review from ordishs

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@galt-tr @icellan @oskarszoon